-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change how actual DB version is checked #34889
Change how actual DB version is checked #34889
Conversation
32f1b6e
to
f82e09c
Compare
That's right, and I agree that's problematic, but that's a pre-existing behavior, and in some cases it might be working just fine... What you're suggesting basically boils down to strictly preventing users from connecting to DBs below the default (minimum supported) version. That makes sense, but that's also a breaking change, one that I wanted to avoid when I originally added support for So IMO we need to:
WDYT, @gsmet? |
BTW does anyone know how to add @marko-bekhta to an allow-list so that workflow runs are automatically allowed for his PRs? We can trust him :) |
This comment has been minimized.
This comment has been minimized.
With
added to the application properties, the app starts and ORM only logs the warning:
So the user can technically proceed on their own risk 😃 |
Ok, thanks. Well then I suppose that's fine to break this in 3.3 (probably not in 3.2 though, let's not backport this). I suggest that you:
|
@marko-bekhta any chance you could finalize this? It doesn't look like a lot of work. @yrodiere Marko will be added automatically to the allow list when more than 10 PRs of his have been merged. Maybe we should have a list in the config file though to bypass this check for people who are trusted from the get go. |
@gsmet yes sir 😃 thank you for the reminder. I'll try to make this ready for the next week.
Noted 😃, need to add more useful things to Quarkus 😃 🙈 |
f82e09c
to
0172a81
Compare
I've updated the note in the documentation. As for the migration notes, I'm not sure I can add those, so here's the text I've came up with for it: === Hibernate ORM
The Hibernate ORM Quarkus extension changed the way it verifies the database version
to provide more useful feedback from Hibernate ORM itself. In particular, to let the user know if
the version of the database being used is no longer considered as https://in.relation.to/2023/02/15/hibernate-orm-62-db-version-support/[supported by Hibernate ORM].
This change should only affect a supported database subset and only for older versions:
- MariaDB older than `10.6`
- MySQL older than `8`
- Oracle Database older than `12`
- Microsoft SQL Server older than `13 (2016)`
If the database cannot be upgraded at the time, it is still possible to use it after explicitly setting
the database version and, in some cases of very old databases that only have a community version of the dialect, by setting the dialect.
Refer to https://docs.jboss.org/hibernate/orm/6.3/userguide/html_single/Hibernate_User_Guide.html#database-dialect[database dialects] for more information. I took the DBs and versions from the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though you might want to check whether there's dead code in RecordedConfig
now (is the db version still needed there?).
Now we just need to think about adding this change to the migration guide for Quarkus 3.5.
If that's fine with you, I'll add this slightly modified version to the migration guide:
|
🎊 PR Preview f0d4d20 has been successfully built and deployed to https://quarkus-pr-main-34889-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
0172a81
to
1f77223
Compare
This comment has been minimized.
This comment has been minimized.
1f77223
to
01b96bc
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Oh my... We added the notes to the migration guide but never merged the PR... |
The minimum Derby version was bumped to 10.15.2 in ORM: https://hibernate.atlassian.net/browse/HHH-15203 Can we bump the version in Quarkus? As for the Kafka test... all ok locally 😕🤷🏻♂️ |
+1, will do |
Because Hibernate ORM 6.3+ doesn't technically support Derby 10.14: https://hibernate.atlassian.net/browse/HHH-15203 Right now Quarkus and Hibernate ORM will start even if using Derby 10.14, but: * there will be a warning on startup * some Hibernate ORM features may not work correctly Also, we're adding stricter checks in quarkusio#34889, so startup will start failing if people are still on an older version of Derby.
|
Because Hibernate ORM 6.3+ doesn't technically support Derby 10.14: https://hibernate.atlassian.net/browse/HHH-15203 Right now Quarkus and Hibernate ORM will start even if using Derby 10.14, but: * there will be a warning on startup * some Hibernate ORM features may not work correctly Also, we're adding stricter checks in quarkusio#34889, so startup will start failing if people are still on an older version of Derby.
Because Hibernate ORM 6.3+ doesn't technically support Derby 10.14: https://hibernate.atlassian.net/browse/HHH-15203 Right now Quarkus and Hibernate ORM will start even if using Derby 10.14, but: * there will be a warning on startup * some Hibernate ORM features may not work correctly Also, we're adding stricter checks in quarkusio#34889, so startup will start failing if people are still on an older version of Derby.
Because Hibernate ORM 6.3+ doesn't technically support Derby 10.14: https://hibernate.atlassian.net/browse/HHH-15203 Right now Quarkus and Hibernate ORM will start even if using Derby 10.14, but: * there will be a warning on startup * some Hibernate ORM features may not work correctly Also, we're adding stricter checks in quarkusio#34889, so startup will start failing if people are still on an older version of Derby.
01b96bc
to
693bfdb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased on top of #41084 (which was merged to main).
LGTM, let's see if CI agrees.
Reminder: we'll need to adjust migration guides once this gets in, see #34889 (comment)
Status for workflow
|
Status for workflow
|
Status | Name | Step | Failures | Logs | Raw logs | Build scan |
---|---|---|---|---|---|---|
✖ | JVM Tests - JDK 17 | Build |
Failures | Logs | Raw logs | 🔍 |
✔️ | JVM Tests - JDK 21 | Logs | Raw logs | 🔍 |
You can consult the Develocity build scans.
Failures
⚙️ JVM Tests - JDK 17 #
- Failing: integration-tests/jpa-mssql
📦 integration-tests/jpa-mssql
✖ Failed to execute goal io.fabric8:docker-maven-plugin:0.44.0:start (docker-start) on project quarkus-integration-test-jpa-mssql: I/O Error
Flaky tests - Develocity
⚙️ JVM Tests - JDK 21
📦 integration-tests/reactive-messaging-kafka
✖ io.quarkus.it.kafka.KafkaConnectorTest.testFruits
- History
Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds.
-org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds.
at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
at io.quarkus.it.kafka.KafkaConnectorTest.testFruits(KafkaConnectorTest.java:63)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
I will let you add an item to the migration guide here: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.12 . |
Done, and I also removed the section that was mistakenly added to the 3.5 migration guide. |
@michelle-purcell @rolfedh See ^. There was a section in the migration guide for Quarkus 3.5 that looked a bit like this and that essentially said database version checks were stricter now. |
Thanks for this update, @yrodiere. |
Because Hibernate ORM 6.3+ doesn't technically support Derby 10.14: https://hibernate.atlassian.net/browse/HHH-15203 Right now Quarkus and Hibernate ORM will start even if using Derby 10.14, but: * there will be a warning on startup * some Hibernate ORM features may not work correctly Also, we're adding stricter checks in quarkusio#34889, so startup will start failing if people are still on an older version of Derby.
Because Hibernate ORM 6.3+ doesn't technically support Derby 10.14: https://hibernate.atlassian.net/browse/HHH-15203 Right now Quarkus and Hibernate ORM will start even if using Derby 10.14, but: * there will be a warning on startup * some Hibernate ORM features may not work correctly Also, we're adding stricter checks in quarkusio#34889, so startup will start failing if people are still on an older version of Derby.
Hi, it looks like this check creates problems when running H2 in compatibility mode with Postgres. Hibernate requires Postgres 12.0, and so QuarkusStaticInitDialectFactory checks, whether the running db has version 12.0 or more. Since the DB is H2 which currently has version 2.2, the factory presumes, that we are trying to connect to postgress 2.2, and fails. Is there some workaround for this? |
@fedinskiy I think we decided that using H2 in compatibility mode was not really something people should do as it comes with a lot of quirks. With Testcontainers today, there's really not much value doing that. |
Can be the check disabled? We have coverage using H2 because we have one test-suite that needs to run in no-docker environment. |
@fedinskiy we use H2 with |
I'm afraid if you are testing H2 with PostgreSQLDialect, you are effectively testing something that was never intended to work. Passing tests with such a configuration are not proof that a real application using PostgreSQL would work properly.
From what I understand, this means that H2 in PostgreSQL compatibility mode reports being PostgreSQL 2.2. At least, that's what Hibernate ORM would detect, even outside of Quarkus.
Possibly. I think you can acknowledge to Quarkus that you really want to work with "PostgreSQL version 2.2" (which I'm not sure has ever existed, but that's beside the point):
That'll give you the default behavior of Hibernate ORM, which is logging a warning on startup, and then behaving in a totally unspecified way, because Hibernate ORM isn't designed to work with PostgreSQL 2.2. But, as I mentioned above, that's a bad idea:
|
@yrodiere sorry for not explicitly closing the question earlier. |
This came up while looking into #34754
In that scenario, the actual DB version is 5.5, but because of the
DialectVersions.Defaults
, the version is set to 8.0 😨🥲, which I guess may lead to problems in runtime...And
checkActualDbVersion()
call never got further than the first if statement. It seems safe to say thatbuildTimeDbVersion
is whatever the driver version got initialized to...With these changes, the app would first fail with:
and then if the db-version is configured:
ORM will warn the user about the unsupported version.